fix: make rate limiter checks independent of window size (#125) - #130
Merged
Merged
Conversation
isAllowed() filtered the caller's full timestamp window on every call, including denied requests and pre-auth traffic, so per-request cost grew linearly with RATE_LIMIT_REQUESTS and collapsed quadratically under load. Timestamps are sorted ascending, so stale entries form a prefix: take an O(1) fast path when the newest is fresh, otherwise binary search the first fresh entry (O(log n)) and count the window without filtering or copying. Drop the stale prefix only once it dominates the array so the copy stays amortized O(1) per recorded request. All-stale entries are still removed, and periodic cleanup and eviction semantics are unchanged.
The atomic-snapshot rewrite in #122 left persist.ts at 78-79% on Stryker, failing the per-file threshold for any PR that touches tests. Kill the surviving mutants through the public FileStore interface: full truncation of stale temp content, rethrowing unusable-temp-path errors, temp cleanup on failed rename, and multi-byte reassembly across 4096-byte read boundaries.
#128) * fix: return 400 for JSON nested beyond the parser's stack depth (#123) Root cause: V8 applies a JSON.parse reviver recursively, one stack frame per nesting level. The enqueue reviver (unsupported-number check) made bodies nested ~3,100+ levels deep throw RangeError: Maximum call stack size exceeded. enqueueErrorResponse only maps SyntaxError, so the RangeError escaped as an uncaught 500. Plain JSON.parse and JSON.stringify both handle 100,000+ levels, so the parse step was the only point of failure. parseJsonBody now converts a RangeError raised by JSON.parse into a SyntaxError. The request gets the existing 400 "Invalid JSON" response and the Queue is not changed. The catch covers only the parse call, so RangeErrors from anywhere else still surface as 500s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor: map parser depth overflow via a local error type Throwing `new SyntaxError` added a module dependency and pushed handler.ts to the CouplingBetweenObjects limit (13) in the production quality gate. A local JsonNestingTooDeepError, mapped to the same 400 "Invalid JSON" response, follows the existing UnsupportedNumberError pattern and keeps coupling at 12. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: restore persist.ts mutation coverage below the 80% gate The atomic-snapshot rewrite in #122 left persist.ts at 78-79% on Stryker, failing the per-file threshold for any PR that touches tests. Kill the surviving mutants through the public FileStore interface: full truncation of stale temp content, rethrowing unusable-temp-path errors, temp cleanup on failed rename, and multi-byte reassembly across 4096-byte read boundaries. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Decode request bytes with a fatal UTF-8 decoder so malformed JSON strings return the existing 400 Invalid JSON response instead of being replaced with U+FFFD. Add HTTP seam coverage for rejection, no enqueue, and valid UTF-8 round trips.
Parse request bodies natively, then scan the original JSON source to validate number literals. This preserves exact-number rejection while avoiding a reviver callback for every value in number-dense payloads. Add public handler regressions for exact integers, nested metadata, JSON strings, and the explicit nesting limit.
…leet/worktree-queue-125
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #125
Diagnosis
RateLimiter.isAllowed()(src/rate_limiter.ts) rantimestamps.filter(ts => ts > cutoff)on every call — an O(window) scan and allocation per request, paid before authentication (rate limiting wraps auth) and even for requests that end up denied. Filling one caller's window and issuing denied requests therefore scaled linearly withRATE_LIMIT_REQUESTS, collapsing quadratically under sustained load.Confirmed with a live benchmark (
docs/exploratory-testing/2026-09-21-queue/perf_repro.ts), matching the triage timings on the issue:Fix
Timestamps are appended in non-decreasing order, so stale entries always form a prefix of each caller's array. Instead of filtering on every call:
freshCount = length - firstFresh, without copying the window.firstFresh * 2 >= length), so the copy stays amortized O(1) per recorded request.cleanupStaleEntries(),maxTrackedIPseviction (last element = max timestamp still holds), the sliding-window boundary (ts > cutoff), client identity, and the 429 contract are unchanged.Tests
>=, not>).Note: persist.ts mutation coverage
The second commit is CI infrastructure repair, not part of the rate-limit fix: the atomic-snapshot rewrite in #122 left
src/persist.tsat 78–79% on the Stryker mutation gate (80% per-file threshold), which has kept main'sMutation testing (Stryker)job red since Sep 20. Any PR touching tests runs the full mutation suite, so this PR cannot go green without restoring those kills. The new tests kill the survivors through the publicFileStoreinterface only: full truncation of stale temp content, rethrowing when the temp path is unusable, temp cleanup on failed rename, and multi-byte reassembly across 4096-byte read boundaries.